New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simplify and speed up VulnerabilityMetricsUpdateTask #2481
Simplify and speed up VulnerabilityMetricsUpdateTask #2481
Conversation
Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
src/main/java/org/dependencytrack/tasks/metrics/VulnerabilityMetricsUpdateTask.java
Show resolved
Hide resolved
Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
Test failure:
|
@valentijnscholten I think H2 is stumbling here because |
Yeah I'm testing a fix and adding some tests. At first it looked like wrong SQL as the "official" way is not |
Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
4090b10
to
e98ab7e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great optimization, thanks @valentijnscholten!
I made some suggestions on how the code could be simplified even more, but those are really just of cosmetic nature.
Question: The old code had some logic in place to keep the old database ID values in place for each Year-Month combination. This PR just removes old metrics and creates new ones to keep it simple. Not sure if there are any clients/users that expect the ID to be constant? I hope they look at the Year-Month combination...
To be honest I don't think there was a specific reason for keeping it, other than perhaps the assumption that deleting and inserting it all would be more expensive that updating.
IDs are never exposed to clients, so this change should not matter to users.
src/main/java/org/dependencytrack/persistence/MetricsQueryManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/dependencytrack/persistence/MetricsQueryManager.java
Outdated
Show resolved
Hide resolved
src/main/java/org/dependencytrack/tasks/metrics/VulnerabilityMetricsUpdateTask.java
Outdated
Show resolved
Hide resolved
src/main/java/org/dependencytrack/tasks/metrics/VulnerabilityMetricsUpdateTask.java
Outdated
Show resolved
Hide resolved
src/main/java/org/dependencytrack/tasks/metrics/VulnerabilityMetricsUpdateTask.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
d2af48d
to
0f8f2f0
Compare
Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
…s fetch group Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks @valentijnscholten!
Any more pending changes you were planning to add, or is this ready to merge from your side?
I think it's good to go. |
…#2481) * Simplify and speedup VulnerabilityMetricsUpdateTask Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com> * Simplify and speedup VulnerabilityMetricsUpdateTask Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com> * Simplify and speedup VulnerabilityMetricsUpdateTask Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com> * Simplify and speedup VulnerabilityMetricsUpdateTask Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com> * Simplify and speedup VulnerabilityMetricsUpdateTask with safe aliases Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com> * Simplify and speedup VulnerabilityMetricsUpdateTask suggested changes Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com> * Simplify and speedup VulnerabilityMetricsUpdateTask suggested changes Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com> * Simplify and speedup VulnerabilityMetricsUpdateTask remove superfluous fetch group Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com> * Simplify and speedup VulnerabilityMetricsUpdateTask remove casts Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com> --------- Signed-off-by: Valentijn Scholten <valentijnscholten@gmail.com>
Description
This PR simplifies the
VulnerabilityMetricsUpdateTask
by letting the database handling the counting. The runtime is reduced from ~50 seconds to about 4 seconds.Addressed Issue
I noticed the
VulnerabilityMetricsUpdateTask
was taking around 50 seconds to complete on my instance. The only thing that task does is to count the vulnerabilities that were published each year/month. The old code was loading all ~300k vulnerabilities into memory, in batches of 500. So this meant around 600 queries and 300000 object instantiations and other Datanucleus "overhead".The PR now performs 2 queries to get all the counts from the database. Then it does some juggling to match up the
published
andcreated
fields. And then the old metrics are replaced by the new ones.Additional Details
During the refactoring I found out this data is not even used by the DT front-end, so it might not be on top of the priority list. But it is exposed through the API. Either way, I had fun optimizing it. First time doing something with Datanucleus as well as Java Streams.
Question: The old code had some logic in place to keep the old database ID values in place for each Year-Month combination. This PR just removes old metrics and creates new ones to keep it simple. Not sure if there are any clients/users that expect the ID to be constant? I hope they look at the Year-Month combination...
Checklist